Create conformance test for scs-0214-v1 - #486
Conversation
cah-hbaum
added
Container
mbuechse
left a comment
mbuechse
left a comment
There was a problem hiding this comment.
A few things that I think should be addressed. If you are time-constrained, I offer to assist.
| import yaml | ||
|
|
||
|
|
||
| logging_config = { |
There was a problem hiding this comment.
This seems kinda redundant given that the same info is in the yaml file?
There was a problem hiding this comment.
This is there if a yaml file isn't provided.
There was a problem hiding this comment.
I know. Then it should just be provided (and the fallback can be way more minimal)
There was a problem hiding this comment.
Its already more minimal than the config, but maybe it can be minimized more(?) Not sure though.
I fixed some small artifacts I found in the config.
There was a problem hiding this comment.
Why should the user be able to configure the logging in the first place? I would actually remove the -c/--config option and the yaml file completely.
There was a problem hiding this comment.
Let's come back to this point later. It's not mission critical.
|
Added a fixup containing the mentioned changes. I hope I didn't forget anything @mbuechse Hint: I couldn't test this right now, I got some problems with my K8s clusters. |
martinmo
left a comment
martinmo
left a comment
There was a problem hiding this comment.
Good work, thanks Hannes. But for me, some of the code was hard to understand/follow and I left some suggestions for improvement.
Additionally, with manual testing I could only verify that the following cases have the expected conformance test outcome:
- test with a minikube which is too small ("ERROR: Node is not labelled as a control plane node" and exit code
2) - test with a k8s cluster with only one region ("WARNING: There seems to be no distribution across multiple regions or labels aren't set correctly across nodes." and exit code
0, i.e., conforming)
It would be nice if you could provide some test examples. If you reorganize the code in a way that the labels-by-node list can come from another source and not only from actually calling get_k8s_cluster_labelled_nodes(…) this would be easier to review.
| import yaml | ||
|
|
||
|
|
||
| logging_config = { |
There was a problem hiding this comment.
Why should the user be able to configure the logging in the first place? I would actually remove the -c/--config option and the yaml file completely.
I forgot to mention that, to me, the testing logic seems to be ok. Still, it would be good to have an easy way to verify the different scenarios, without having access to different kinds of clusters. |
I agree with you on this, we can probably look into this later on. (Sadly not until tomorrow). I also fixed/updated/changed most of the things you mentioned in the other comments, so you can recheck tomorrow =) Edit: Also in response to #486 (comment), in a previous test, we wanted to provide a possibility to configure the logging however it is necessary. You could now use the same config from that test here. |
martinmo
left a comment
martinmo
left a comment
There was a problem hiding this comment.
LGTM & we agreed via chat to add tests for the tests later.
This provides a basic node distribution test only operating based on the labels of the kubernetes cluster provided. A test will come back negative, if no distribution according to the labels can be detected. Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>
The test section of the standard also needs to be updated, even if the standard is already stabilized. Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>
4f85444 to
f6e34d9
Compare
3 participants
As discussed in #481, we need to create a conformance test for SCS-0214-v1. This PR does that and also includes a separate commit containing the necessary changes for the
Conformance Testssection of the standard. If we don't want to include this, the commit will be split up into a separate PR.Resolves SovereignCloudStack/issues#477